Add calendar aggregate commands - #21

(withdrawn — superseded by a corrected review on this PR after the maintainer confirmed cross-source merging is intended behaviour)

(withdrawn — superseded by a deeper audit review on this PR)

(withdrawn — superseded by a follow-up review on this PR after the address-findings + patch-coverage commits)

Review (round 4, post fix commits): all 3 blocking findings resolved + 4 deeper non-blocking notes

The fa984ee1 ("address aggregate review findings") and b86d269f ("raise aggregate patch coverage") commits address every blocking finding from the prior strict round, and most of the non-blocking suggestions. Going through them point by point:

Prior findings — resolution status

• B1 — _HTTP_STATUS_RE false-positives ✅ resolved. The regex at animedex/agg/_fanout.py:35–45 now requires a status-introducing token (HTTP, status, returned, response, or a backend name) before the 3-digit number. The new test test_http_status_requires_status_context in test/agg/test_fanout.py pins the false-positive cases (limit=200 reached, etc.) to None and the true-positive cases to the correct status.

• B2 — _to_common_anime silent except ✅ resolved. _to_common_anime now delegates to _to_common_anime_with_diagnostic at line 790. The except is narrowed from bare Exception to (ValueError, AttributeError, KeyError); the failure is logged at logging.DEBUG; a structured diagnostic ({backend, id, reason: "to-common-failed", message}) is returned alongside None. AggregateResult.merge_diagnostics is the new envelope field that surfaces every such diagnostic. test_merge_season_items_reports_to_common_failures covers the path.

• B3 — ids.setdefault silent conflict ✅ resolved with a two-layer defence: _group_has_external_id_conflict filters at grouping time so disagreeing rows never enter the same group, and _set_id at line 1217 raises ValueError with a precise message if the grouping-layer check ever lets a conflict slip through. _external_id_conflicts returns structured {key, left_backend, left_value, right_backend, right_value} records for downstream consumers. test_merge_season_items_splits_external_id_conflicts covers the path.

• S1 — dual transliterator justification ✅ resolved. _title_key_variants now carries a substantial docstring with concrete CJK / Hangul examples (怪獣８号 → GuaiShou8Hao via anyascii vs Guai Swu 8Hao via unidecode; 마녀와 야수 produces different word boundaries). The commit body for fa984ee1 adds the corresponding requirements.txt justification paragraph naming each new dependency's role. Also picked up python-dateutil as a fourth dep, justified by the schedule timezone string support — fine addition.

• S2 — magic threshold calibration ✅ resolved. The calibration note block above _MERGE_THRESHOLD = 70 at animedex/agg/calendar.py:37 states the recall and precision targets (>= 95% / >= 99%), the corpus the thresholds were tuned against, the directional trade-off when widening or narrowing, and the workflow for re-running tools/merge_eval/evaluate_rule.py.

• S3 — _choose_merged_title anilist primary ✅ resolved. The function at line 998 has a docstring stating the preference order and the rationale ("AniList's romaji/native/English title block is the most consistent title schema across the 2010-2025 season corpus").

• S4 — _normalise_items silent shape wrap ⚠️ partially resolved. The function at animedex/agg/_fanout.py:52 now explicitly supports dict-shaped envelopes by reading value["items"] or value["data"], and raises ApiError(reason="upstream-shape") when neither key is present on a dict. But the final fall-through at line 75 (return [value] for objects without a .rows attribute) is still a silent wrap. See S_new3 below.

• S6 — fixture corpus README ✅ resolved. test/fixtures/aggregate/season_matrix/README.md documents the regeneration workflow including tools/merge_eval/build_candidates.py invocation, the manual adjudication steps via build_adjudication_inputs.py + combine_adjudication.py, and proxy-credential hygiene.

• S5 — O(N²) merge perf not addressed (was optional; still optional).

• S7 — pytest.mark.slow for adjudication test not addressed (was optional; still optional).

New findings from the deeper audit

The fix commits land a lot of new code: 882 added lines in calendar.py, a fresh animedex/utils/timezone.py module (152 lines), 499 lines in render/tty.py, and a tools/fixtures/prewarm_aggregate_cache.py. A fresh pass across that surface surfaces four more non-blocking items.

• S_new1 — merge_diagnostics envelope field has no TTY stderr surface. The JSON envelope correctly carries merge_diagnostics when a row falls out of merge analysis, and the CLI's JSON path renders it. But _report_failures at animedex/entry/aggregate.py:58 only iterates over result.failed_sources (per-source upstream failures) and emits one stderr inform line per failed source. It does not iterate result.merge_diagnostics, so a TTY user whose season command encountered to_common() failures (a real bug in a backend's projection, schema drift, etc.) sees the rendered rows but no stderr signal that other rows were dropped from merge analysis. This is asymmetric with the per-source-failure inform path: source-level failures get stderr inform; row-level merge skips do not. Action: extend _report_failures to also emit one stderr inform line per merge diagnostic, e.g. "merge diagnostic: anilist:154587 fell back to passthrough (to_common-failed: AttributeError: ...); continuing". The body counts partial-failure semantics that keep healthy rows on stdout and emit stderr inform as the §0 contract for fan-out; the same contract should apply to merge-stage failures one layer deeper.

• S_new2 — _set_id raises ValueError but no caller catches it. The raise at animedex/agg/calendar.py:1224 is the second defensive layer behind _group_has_external_id_conflict. The first layer (grouping-time pre-check) compares record.ids key sets and should keep _set_id from ever seeing a conflict — but _set_id operates on a wider key space than _external_id_conflicts examines: it also writes IDs derived from record.id's "prefix:value" split (line 1235–1238) and from per-backend fallback. If a rich-model mapper ever produces inconsistent records (e.g. record.ids = {"anilist": "999"} while record.id = "anilist:154587"), _set_id raises and the exception propagates all the way up: _merge_season_items does not catch it (line 1283 list comprehension), season() does not catch it (line 1338), and animedex/entry/aggregate.py:43 only catches ApiError. End state: an uncaught ValueError traceback to the CLI user. Action: either (a) wrap the list comprehension in _merge_season_items with a try/except that converts the raise into a structured merge_diagnostics entry and re-splits the group into single-source entries, or (b) replace the raise in _set_id with a record into a conflicts list that flows into MergedAnime.id_conflicts (consistent with the structural-conflict shape _external_id_conflicts already returns). Either resolution keeps the failure observable through the envelope and prevents a CLI traceback.

• S_new3 — _normalise_items final fall-through silently wraps unknown shapes. Line 75 of animedex/agg/_fanout.py reads return [value] for any source return value that isn't None, list, tuple, dict, or an object exposing .rows. The dict path now correctly raises upstream-shape; the unknown-object path still silently wraps a single object as a single-row list. Today's backends never hit this path, but a future backend whose return value is a custom dataclass with no .rows accessor will become a silent single-row source. Action: either (a) raise ApiError(reason="upstream-shape", message=f"aggregate source returned unsupported shape: {type(value).__name__}"), or (b) keep the wrap but add a comment naming the contract the project relies on (e.g. "All current backends return list-of-Anime or a wrapper with .rows; this fall-through accepts a single scalar as a one-row source, which is the legacy shape from _jikan_rows returning early; revisit when a new backend lands"). The (a) form is cleaner and consistent with the dict-shape raise immediately above.

• S_new4 — unidecode not listed in PyInstaller hidden imports / package datas. tools/generate_spec.py:99 explicitly hides anyascii._data because anyascii lazy-loads its transliteration tables under a resource-only sub-package the PyInstaller static analyser doesn't reach. unidecode has the same lazy-load pattern (from Unidecode import x0041 per code-point block, via __import__), but no equivalent hidden-import entry exists in the spec. The collect_submodules('animedex') call at the spec template's top picks up animedex's own submodules but not unidecode's. Today's make build && make test_cli passes — likely because PyInstaller's stdlib collection happens to grab unidecode submodules through its own scan — but the asymmetry with anyascii is fragile: a future PyInstaller upgrade that tightens dynamic-import handling could silently break unidecode("怪獣") in the frozen binary. Action: add "unidecode" and "unidecode.util" to HIDDEN_IMPORTS and "unidecode" to PACKAGE_DATAS (alongside anyascii), with a comment explaining the parallel justification. Verify after by running make build && make test_cli and grepping the frozen-binary directory tree for Unidecode/x*.py data files.

Process note (third occurrence; recording only)

Issue #18's 06:39Z self-reply ("Maintainer sign-off received in the working thread") remains the same 4-minute self-handshake pattern PR #17 first flagged. This is the third PR where the pattern appears. The maintainer eventually confirmed direction after the fact in both cases, so no harm done — but the §15.5 sign-off contract is not being honoured, and the next occurrence may not have the lucky direction-was-correct outcome. Worth tightening §15.5's wording in a separate hygiene PR so "Sign-off received" requires either a maintainer reply (someone other than the proposer) with a known marker, or an explicit out-of-band record (linked elsewhere in the issue trail). Not actionable on this PR.

Conclusion

The fix completeness is high — all three prior blocking findings are resolved with matching regression tests, the dual-transliterator and threshold-calibration docstrings land cleanly, and the fixture-corpus README closes the maintenance trail. The four deeper non-blocking items above are stage-two polish: merge_diagnostics deserves a stderr surface for symmetry with source-level partial-failure (S_new1), the _set_id raise path needs a catch boundary or a structural-conflict alternative (S_new2), the _normalise_items final fall-through should raise or be explicitly documented (S_new3), and the PyInstaller spec should hide unidecode data tables like anyascii already does (S_new4).

S_new2 is the most pressing because it's a latent crash path that bypasses the typed-error contract. The other three are robustness/symmetry improvements that can land in this PR or a follow-up. None individually block the merge — combined, they're the right shape for one more fix commit.

event=COMMENT because GitHub blocks self-REQUEST_CHANGES; treat the framing as a follow-up change request per §15.6.

— narugo1992

Follow-up review on c2ae80f7 + 3316503f + 4c58b12b — all four v4 findings resolved, ready to merge

The three new commits address every non-blocking item from the prior round, often with extras I didn't ask for. This is the cleanest fix round on this PR so far.

S_new1 — merge_diagnostics no TTY stderr surface — ✅ resolved

animedex/entry/aggregate.py:66 introduces _report_merge_diagnostics, called from _finish immediately after _report_failures. The implementation distinguishes the two diagnostic kinds explicitly:

• external-id-conflict → "merge diagnostic: {backend}:{id} kept with external id conflict ({message})" (the row is preserved with conflicts visible)
• everything else → "merge diagnostic: {backend}:{id} dropped from merge analysis ({reason}: {message}); kept as passthrough row" (the row falls back to passthrough)

Two regression tests pin both branches (test_merge_diagnostics_are_reported_to_stderr for the to-common-failed shape, test_external_id_conflict_diagnostics_are_reported_to_stderr for the conflict shape). The §0 inform-not-gate symmetry with _report_failures is now complete.

S_new2 — _set_id raise propagating uncaught — ✅ resolved (structural alternative path chosen)

_set_id no longer raises. The signature gains backend and source keyword parameters; on conflict the helper appends a structured record to a function-local id_conflicts list and returns. MergedAnime gains an id_conflicts: List[Dict[str, Any]] = Field(default_factory=list) field carrying the conflict records through the envelope. _merge_season_items translates each conflict into a merge_diagnostics entry with reason: "external-id-conflict" so the stderr surface (S_new1) picks it up. The CLI now never sees an uncaught ValueError; the structural envelope is the single source of truth for disagreement signals.

This is the (a) alternative I suggested in v4 — cleaner than catch-at-boundary because the conflict structure unifies with _external_id_conflicts's existing shape. The regression test test_merge_season_items_reports_internal_id_conflicts_without_traceback constructs the previously-crashing inconsistent-mapper corner case (record.ids = {"anilist": "999"} vs record.id = "anilist:154587") and asserts (i) no traceback, (ii) the conflict surfaces structurally on the envelope, (iii) the diagnostic message reaches merge_diagnostics.

S_new3 — _normalise_items silent fall-through — ✅ resolved

animedex/agg/_fanout.py:75 now raises ApiError(reason="upstream-shape", message="aggregate source returned unsupported shape: {type_name}") for any value that isn't None, list, tuple, dict, or .rows-bearing object. The test test_normalises_none_tuple_dict_and_rows_object is renamed (the _and_scalar part is dropped) and a new assertion checks that _normalise_items("x") raises upstream-shape with the expected message.

S_new4 — unidecode PyInstaller asymmetry — ✅ resolved with bonus defence in depth

tools/generate_spec.py:99 adds "unidecode" and "unidecode.util" to HIDDEN_IMPORTS, and "unidecode" to PACKAGE_DATAS, both with comment blocks explaining the lazy-load justification (parallel to anyascii). test/tools/test_generate_spec.py is a new file that asserts the spec generator carries both transliterators' data references — a regression-test catch in case a future spec refactor removes one accidentally.

The bonus: animedex/diag/selftest.py:409 adds _smoke_unidecode() that asserts both an ASCII case (unidecode("Pokémon") == "Pokemon") and a Japanese kana case (unidecode("ソードアート") == "so-doa-to"). Combined with the existing _smoke_anyascii at line 392 testing 怪獣８号 → GuaiShou8Hao, every transliterator the project depends on now has a frozen-binary-friendly smoke check. This goes beyond what S_new4 asked for and gives the project a runtime guarantee that PyInstaller upgrades won't silently break either library.

Extras I didn't ask for but appreciate

• 3316503f — anilist.airing_schedule filters out None variables before sending to GraphQL. Previously the function passed {"mediaId": None, "notYetAired": None, ...} to _gql; now it builds the variable dict and drops None entries. AniList tolerates either shape, but a clean request body makes wire-level inspection easier and avoids ambiguity if AniList ever distinguishes "not specified" from "null". A regression test (test_airing_schedule_omits_unset_filters in the anilist suite) pins the wire shape.
• 4c58b12b — schedule test stability. The previous rsps.calls[0] was positional-ordering-dependent on which mocked response fired first; _anilist_graphql_requests(rsps) filters by method == "POST" + url == "https://graphql.anilist.co/" and asserts len(...) == 1 before reading. Replaces a brittle index assumption with a structural lookup. Good test-hygiene improvement.

Nit-level observations (truly optional, not actionable for this PR)

These are micro-issues that don't justify another fix round; flagging only so they have a recorded home.

• _to_common_anime_with_diagnostic at animedex/agg/calendar.py:790 returns (None, None) (no diagnostic) when item lacks to_common entirely or its to_common() returns a non-Anime value. The narrowly-typed exception branch above does emit a diagnostic, but the type-mismatch branch silently falls through to passthrough. Today every rich-model returns Anime from to_common, so this is unreachable in practice; if a future backend's to_common() ever returns a wrong type, the silent drop reappears. A trivial improvement would be to emit a diagnostic like {"reason": "to-common-returned-non-anime"} in that branch — but it's defending against a contract violation that the rich-model lossless test suite would also catch. Not worth a fix commit.
• MergedAnime.id_conflicts is a top-level field and core["id_conflicts"] is set to the same list inside the compact core dict at line 1252. JSON consumers read the same data twice; harmless but slightly redundant. If you happen to be in the file for an unrelated reason, picking one location is cleaner.

Process note (final reinforcement, recorded only)

No new self-handshake on this round; the conversation flow on issue #18 has been quiet since the maintainer's direction confirmation. The §15.5 wording-tightening proposal stands as a future hygiene item but is unrelated to this PR.

Conclusion

Ready to merge. The fix completeness across the four review rounds on this PR has gone from "spec-violating + uncaught crash paths + silent drops" through "structurally correct but with documentation gaps and asymmetric surfaces" to "every diagnostic surfaces both as structured envelope field and as TTY stderr inform; every silent fall-through path either raises or is documented; both transliterator backends have frozen-binary smoke coverage". That's the right shape for this PR's complexity.

event=COMMENT because GitHub blocks self-APPROVE; treat the framing as approval-in-spirit per §15.6.

— narugo1992

Follow-up review on 73fa6bb6 — schedule timeline render is a clean visual polish, ready to merge

This commit reshapes the schedule TTY output into a left-gutter timeline (Unicode │ between time-of-day and the row title, continued vertically across each day's rows) and adds a graceful ASCII fallback (|) when the destination stream cannot encode the Unicode box-drawing character. It also threads an optional stream parameter through render_tty / _to_tty_text / _format_schedule_calendar_tty so callers can opt the renderer into stream-aware glyph selection.

Audit

Looked at the diff line by line and traced the new code paths against the existing renderer / dispatcher seams. Everything checks out:

• _stream_supports_text(stream, text) boundary semantics are correct. stream is None returns True; encoding is None returns True; a broken codec (e.g. "no-such-codec-xyz") returns False because text.encode(encoding) raises LookupError, which the except (LookupError, UnicodeEncodeError) catches. Verified empirically against the helper in a fresh Python shell. The Unicode-by-default-when-uncertain stance is the right one for the render_tty direct-call surface that doesn't pass a stream — the explicit-stream callers (the two production callers below) carry the actual encoding context.

• All production callers thread stream. animedex/entry/aggregate.py:54 passes sys.stdout; animedex/entry/_cli_factory.py:80 passes its caller-provided stream. render_for_stream at animedex/render/tty.py:966 passes the stream it was called with. The only path that defaults to stream=None is direct render_tty(model) invocations from tests / library users; for those the Unicode default is safe because if the eventual print() target can't encode the char, the user gets a UnicodeEncodeError they can act on (the project's only legitimate avoidance is via the explicit stream-aware path).

• _render_schedule_timeline_tree is a clean reshape. Renders sections into a local io.StringIO, then prepends every line with " " * 6 + timeline + " ". The two-pass approach keeps _render_tree's column math independent of the gutter — important because that helper is also called outside the schedule path. The blank-timeline line between rows (line 851: if index: print(f"{' ' * 6}{timeline}", file=out)) is the right visual continuation.

• ASCII fallback regression test pins the fallback shape. test_calendar_falls_back_to_ascii_timeline_when_stream_cannot_encode_unicode constructs an AsciiStream with encoding="ascii" and asserts (a) 01:00 | First Row appears, (b) the inter-row continuation |\n02:00 | Second Row is present, (c) │ is absent from the output. This is the right shape for the fallback contract.

• Existing tests updated symmetrically. The five existing schedule-TTY tests (test_calendar_* family) all replace the old "01:00 Title" two-space-separated assertions with the new "01:00 │ Title" shape. No assertion was dropped silently.

• The non-schedule render paths are untouched. _format_anime_tty, _format_character_tty, _format_merged_anime_tty, etc. don't take a stream argument and don't need one (they don't use box-drawing glyphs). The single-row _format_airing_schedule_tty also doesn't use the timeline character — it's only used when an AiringScheduleRow is rendered outside a calendar context. That asymmetry is fine: timeline is a per-day grouping convention, not a per-row decoration.

• §14 backref grep returns zero across animedex/agg/, animedex/entry/aggregate.py, animedex/models/aggregate.py, animedex/utils/, and the touched animedex/render/tty.py.

One nit-level observation (not actionable)

_stream_supports_text(None, ...) and _stream_supports_text(stream_with_encoding_None, ...) both return True via separate branches. They are indirectly covered by the Unicode-default assertions in the existing tests (which pass stream=None implicitly), but neither has a direct unit test asserting _stream_supports_text(None, "│") is True. Trivial — the helper is six lines and the existing assertions cover the effect — but if you happen to be in test/render/test_tty.py for an unrelated reason, a one-line assert _stream_supports_text(None, "│") is True would close the symmetry with the existing fallback assertion. Not worth a separate fix commit.

Conclusion

Ready to merge. The timeline render is a real readability improvement (multi-row schedule output now reads as a coherent day-of-week column rather than a flat list), the ASCII fallback handles non-UTF-8 terminals correctly without requiring any caller-side awareness, and the stream-parameter threading is backward compatible. No new issues surfaced from the deep audit.

For the running record: the four review rounds on this PR have brought it from spec-mismatched + uncaught crash paths + silent drops (round 1) through structural fixes for those (rounds 2–3) to TTY symmetry, ASCII-stream graceful degradation, and frozen-binary transliterator smoke coverage (rounds 4–5). The work is solid.

event=COMMENT because GitHub blocks self-APPROVE; treat the framing as approval-in-spirit per §15.6.

— narugo1992